Skip to content

gh-131952: Add color to the json CLI #132126

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

Conversation

tomasr8
Copy link
Member

@tomasr8 tomasr8 commented Apr 5, 2025

Example:

image

I'm open to changing the colors. For now it's green for strings, yellow for numbers and cyan for true/false/null.

Copy link
Contributor

@StanFromIreland StanFromIreland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How much of a performance impact does this have?

@picnixz
Copy link
Member

picnixz commented Apr 5, 2025

How much of a performance impact does this have?

It doesn't matter here. json.tool is more of a visualization tool. If users don't want to colorize the output they can also disable it using the NO_COLOR env. var IIRC. OTOH, we could add an explicit --pretty flag to enable colorization instead of making the default.

@tomasr8
Copy link
Member Author

tomasr8 commented Apr 5, 2025

OTOH, we could add an explicit --pretty flag to enable colorization instead of making the default.

I think we might not need that, can_colorize can already detect most situations where color is not desirable (e.g. not a TTY). You can also set NO_COLOR to turn off colors unconditionally.

@AA-Turner AA-Turner changed the title gh-131952: Add color to the json.tool CLI. gh-131952: Add color to the json.tool CLI Apr 5, 2025
@picnixz
Copy link
Member

picnixz commented Apr 5, 2025

What about CLIs? they would be broken if data is piped?

@tomasr8
Copy link
Member Author

tomasr8 commented Apr 5, 2025

What about CLIs? they would be broken if data is piped?

AFAIU they wouldn't (and local testing confirms that), see this comment from Hugo: #131952 (comment)

@raztd
Copy link

raztd commented Apr 6, 2025

@tomasr8 thank you for taking the time to look into it. couple of notes regarding the keys 1) could you make them a different color from the string values? 2) could you make them bold?

this is what jq does and i think it provides a good ux https://static1.howtogeekimages.com/wordpress/wp-content/uploads/2020/01/5-7.png

@tomasr8
Copy link
Member Author

tomasr8 commented Apr 6, 2025

@tomasr8 thank you for taking the time to look into it. couple of notes regarding the keys 1) could you make them a different color from the string values? 2) could you make them bold?

this is what jq does and i think it provides a good ux https://static1.howtogeekimages.com/wordpress/wp-content/uploads/2020/01/5-7.png

That should be in theory possible, we can distinguish keys from string literals since keys are always followed by :. I will make a couple different color versions and then we can decide which one looks the best.

Lib/json/tool.py Outdated
Comment on lines 53 to 54
with_colors = can_colorize()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable isn't doing much, shall we just call the function directly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may have been too cautious here. Without the variable, the function would be called in a loop and I was worried the result could change in between invocations, though that is really unlikely. If you prefer I can remove it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I applied your other suggestion which already removed it so never mind 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that's a good point: there could be a lot of objs as JSONL files can be quite long. Let's add with_colors back, but just outside the loop.

@python-cla-bot
Copy link

python-cla-bot bot commented Apr 6, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

@hugovk hugovk changed the title gh-131952: Add color to the json.tool CLI gh-131952: Add color to the json CLI Apr 6, 2025
Co-authored-by: Adam Turner <[email protected]>
@tomasr8
Copy link
Member Author

tomasr8 commented Apr 12, 2025

Here's a couple more variants. The first is the current and uses the same colors as Lukasz's syntax highlight PR. The last one uses a different color for the keys.

@hugovk
Copy link
Member

hugovk commented Apr 14, 2025

Comparing some this PR, bat and jq, I find jq the best because it has different colours for key and value strings, and there's better contrast between blue and green, and it looks better on light background:

This PR

imageimage

bat image image
jq image image

@tomasr8
Copy link
Member Author

tomasr8 commented Apr 15, 2025

Here's a version with keys in a different color and a bit better color contrast:

@AA-Turner
Copy link
Member

The green and cyan colours here look bad on the white background, which is the default for Windows Terminal, PyCharm's in-built terminal, etc. Could you darken them?

@tomasr8
Copy link
Member Author

tomasr8 commented Apr 15, 2025

The previous green and cyan were the 'intense' variants. Here's the normal one. Below is also a version where the colors are bold which I think looks even better.

With bold colors:

@AA-Turner
Copy link
Member

I also prefer the bold version, thanks.

A

@tomasr8
Copy link
Member Author

tomasr8 commented Apr 15, 2025

Thanks for your feedback! I think it looks much better now :) I added the bold colors and updated tests. Would you mind taking another look?

@tomasr8 tomasr8 requested a review from AA-Turner April 15, 2025 21:41
@raztd
Copy link

raztd commented Apr 16, 2025

The previous green and cyan were the 'intense' variants. Here's the normal one. Below is also a version where the colors are bold which I think looks even better.

With bold colors:

i like the colors. one small request: could you bold the keys and leave the values regular? that's how jq does it https://static1.howtogeekimages.com/wordpress/wp-content/uploads/2020/01/5-7.png

@tomasr8
Copy link
Member Author

tomasr8 commented Apr 16, 2025

could you bold the keys and leave the values regular?

I think I prefer the bold colors. In my opinion, they are more legible when bold, especially with the light theme.

@raztd
Copy link

raztd commented Apr 16, 2025

could you bold the keys and leave the values regular?

I think I prefer the bold colors. In my opinion, they are more legible when bold, especially with the light theme.

in the last pair of screenshots, i see only the values as bold. the keys look regular. my proposal was to invert those. or do you want to make the keys bold, too? i think that could work

Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

A

Comment on lines +36 to +38
for key in _colors:
if m := match.group(key):
color = _colors[key]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for key in _colors:
if m := match.group(key):
color = _colors[key]
for key, color in _colors.items():
if m := match.group(key):

@hugovk
Copy link
Member

hugovk commented Apr 17, 2025

Looking good. One suggestion, jq uses default colour/uncoloured for numbers:

image

Which is more readable with light background than yellow?

image

@hugovk
Copy link
Member

hugovk commented Apr 17, 2025

Please could you also add this to What's New, and mention the colour env vars, like the other similar entries?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants